I got the tests compiling and running. There are 9 failures with features 'lua' and 'rhai'.

There is a regression in that it doesn't support multiple scripts. I'm trying to get that ready in the 'use-entities' branch, which is compiling now but not running yet if you wanted to take a peek. But maybe this PR needs to cook a little more. I'm gonna take another pass on it tonight.

Ok, I have fixed the regression. Game of life works in all ways for Lua. For Rhai it doesn't seem to work for static scripts.

So as I was trying to make scripts work with a shared context and per entity/script, I realized the split of contexts is actually a big policy decision. I imagine in most cases it'll come down to a few qualities: entity, script, and another thing I'm calling domain. To that I end I wrote this trait:

/// A generic script context provider
pub trait ScriptContextProvider<P: IntoScriptPluginParams> {
    /// Get the context.
    fn get(&self, id: Option<Entity>, script_id: &ScriptId, domain: &Domain) -> Option<&Arc<Mutex<P::C>>>;
    /// Insert a context.
    fn insert(&mut self, id: Option<Entity>, script_id: &ScriptId, domain: &Domain, context: P::C) -> Result<(), P::C>;
    /// Returns true if there is a context.
    fn contains(&self, id: Option<Entity>, script_id: &ScriptId, domain: &Domain) -> bool;
}

My hope is this would allow us to provide the right criteria to choose contexts divisions.

SharedContext

So the shared context is implemented with this:

pub struct SharedContext<P: IntoScriptPluginParams>(pub Option<Arc<Mutex<P::C>>>);

and it ignores the arguments and just provides the same context to everyone.

EntityContext

A per entity context is implemented like this:

/// Stores the script context by entity.
pub struct EntityContext<P: IntoScriptPluginParams>(HashMap<Entity, Arc<Mutex<P::C>>>);

ScriptContext composition

And the resource we make available is a composition:

#[derive(Resource)]
/// Keeps track of contexts
pub enum ScriptContext<P: IntoScriptPluginParams> {
    /// One shared script context
    Shared(SharedContext<P>),
    /// One script context per entity
    ///
    /// Stores context by entity with a shared context as a last resort when no
    /// entity is provided.
    Entity(EntityContext<P>, SharedContext<P>)
}

And if we wanted to we could other provide other policies like by ScriptId or something.

Domain

But what's domain? It's the old ScriptId sort of, maybe?

/// A kind of catch all type for script context selection
///
/// I believe this is what the original ScriptId was intended to be.
pub type Domain = Option<Cow<'static, str>>;

I don't do anything with domain presently, and if you think it's extraneous we can toss it.

My vision for domain was to allow users to clump scripts together a little more haphazardly. Something like the following fantasy code would put scripts into one of two domains:

asset_server.load_with_settings("player.lua", |settings: &mut ScriptSettings| {
   settings.domain = Some("player".into());
});
asset_server.load_with_settings("monster.lua", |settings: &mut ScriptSettings| {
   settings.domain = Some("env".into());
});
asset_server.load_with_settings("effects.lua", |settings: &mut ScriptSettings| {
   settings.domain = Some("player".into());
});

Take away

This code is working and I think it's ready for your attention.

Having spent more time with this code base I'm in even more awe of what you've put together. Let me know if there's anything I can do to help. I'm sorry that this branch is probably going to conflict with 0.16 branch. :(

Further work

I'm going to try to make this branch work with Nano-9 to dog food it in the coming days.

It was quick and painless to switch over to this branch in Nano-9. See the commit.

Domains

I went ahead and fleshed out the Domain idea and dropped the Option from its type, but in almost every parameter it is an Option<Domain> now.

pub type Domain = Cow<'static, str>;

How to select a domain?

I thought about adding a 'domain' field to ScriptAsset, but I ultimately decided against it because a script could be used in multiple domains. Instead I just used a component; no component, no domain.

/// Defines the domain of a script
#[derive(Component)]
pub struct ScriptDomain(pub Domain);

Static scripts

I could allow for static scripts to have a domain but I have left them all as being in no domain. Are static scripts grouped into their own context? Are static scripts a kind of domain? Are they superseded by domains?

Add support for context per script

I added with_domains() and per_script().

impl<P: IntoScriptPluginParams> ScriptContext<P> {
    /// Use a shared script context
    pub fn shared() -> Self {
        Self::Shared(SharedContext::default())
    }
    /// Domain contexts or a shared context
    pub fn with_domains() -> Self {
        Self::Domain(DomainContext::default(), SharedContext::default())
    }
    /// Use one script context per entity
    pub fn per_entity() -> Self {
        Self::Entity(EntityContext::default(), SharedContext::default())
    }
    /// Use one script context per entity
    pub fn per_script() -> Self {
        Self::ScriptId(ScriptIdContext::default())
    }
}

Now that there are 4 choices, I'd suggest we change the enable_shared_context to instead pick a default. If the user wants to change it they do:

app.insert_resource(ScriptContext::shared());

Problem: N scripts produce N callback evaluations even with 1 shared context

I added a Recipients::Domain and patching everything up I dealt with an issue I came across as soon as I started using multiple scripts in BMS. If I have five scripts in a shared context, and I do a callback for _update() with Recipients::All, _update() is called FIVE TIMES! One can argue that's correct behavior, but I found it surprising. It's not surprising when every script is in its own context. I worked around this issue in Nano-9 by just taking note of one ScriptId and only calling that. However, updating the code I found an opportunity to implement a general solution.

Proposal: Evaluate Callbacks on a Per Context Basis

Here is my take for least-surprise: If I have five scripts all in one shared context, and I fire a call to Recipients::All, then I want that call to go to the shared context once.

If I have four scripts in three different contexts, and I fire a call to Recipients::All, then I want that call to be evaluated three times, once in each context.

To facilitate that, I added the following method:

/// A generic script context provider
pub trait ScriptContextProvider<P: IntoScriptPluginParams> {
    // ... 
    /// Hash for context.
    ///
    /// Useful for tracking what context will be returned by `get()` without
    /// requiring that `P::C` impl `Hash` and cheaper too.
    fn hash(&self, id: Option<Entity>, script_id: &ScriptId, domain: &Option<Domain>) -> Option<u64>;
}

What I think is nice about this is it works the same as before if you're placing every script in its own context. But it will also work with scripts, entities, domains, and shared contexts.

Docs?

Would you like some help updating the docs/book for this PR?

I'm prepping this branch for a BMS 0.14.0.

Expecting some real-world use I updated the ScriptContext<P> to allow for the following kinds:

• ScriptContext::shared()
• ScriptContext::per_script()
• ScriptContext::per_entity()
• ScriptContext::domains()
• ScriptContext::shared().with_domains()
• ScriptContext::per_script().with_domains()
• ScriptContext::per_entity().with_domains()

And its default is ScriptContext::per_script() so that it follows the conventions of its preceding versions.

I found some tests failing (and some not even compiling--eek). I'll try to fix them tomorrow.

I keep thinking I'm close to being done, but then cargo unveils some new slew of errors. After much teeth gnashing, I have all 172 tests passing when I run "cargo test" from the root workspace. However, when I go into the core crate's directory and run "cargo test" I get compilation errors from the tests. Shouldn't cargo run all the workspace crates' tests?

Anyway, fixing the test harness unveiled this bug:

• If you added your ScriptAsset w/o AssetServer, it wouldn't work. (Now fixed.)

To fit better with ScriptContextProvider<P> which uses an Option<Entity>, I refactored the instances of Entity::from_raw(0) to be Nones.

I didn't seek out to do this, I think I was bewildered by the test harness not working, but as I was patching things up, I realized that we could load scripts without cloning their contents if they're given via a Handle<ScriptAsset>. So that was just a little performance commit as a treat.

Will continue to work on this until I get all tests to pass. Do you have a magic command you provide on the command line to run all the tests?

try cargo xtask test it's a rust-based makefile essentally!

Progress but cargo xtask test remains unfinished.

It's all compiling now. 3 tests are failing. Unfortunately the fix seems a little hairy. The called_on_right_recipients test in particular requires a change in the event handler to fix.

My workday has been interrupted quite a bit, so I put my partial commits into my "use-handles-dev" branch so it's not distracting in this PR. I changed the event handler and now 5 tests are failing. On it goes.

All tests pass! The game of life example starts, stops with Lua & Rhai with and without static argument.

Assumption Breakage

I had been adding Option<Entity> and Option<Domain> parameters and fields as necessary, but the assumption of one context per script handle was still embedded and becoming awkward. I decided to bite the bullet and break with that assumption with this struct:

pub struct ContextKey {
    /// Entity if there is one.
    pub entity: Option<Entity>,
    /// Script ID if there is one.
    pub script_id: Option<Handle<ScriptAsset>>,
    /// Domain if there is one.
    pub domain: Option<Domain>,
}

It's the new key to context.

/// A generic script context provider
pub trait ScriptContextProvider<P: IntoScriptPluginParams> {
    /// Get the context.
+    fn get(&self, context_key: &ContextKey) -> Option<&Arc<Mutex<P::C>>>;
-    fn get(&self, id: Option<Entity>, script_id: &ScriptId, domain: &Option<Domain>) -> Option<&Arc<Mutex<P::C>>>;
    // ...
}

What's in a Domain?

With the inclusion of Domain in the ContextKey, I wanted it to be Copy if possible, so I altered its defintion to this:

/// A kind of catch all type for script context selection
#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)]
pub struct Domain(u64);

impl Domain {
    /// Create a domain handle.
    pub fn new(hashable: impl Hash) -> Self {
        Domain(DefaultHashBuilder::default().hash_one(hashable))
    }
}

New is Old

This is the new default ScriptContext variant that captures the prior version's behavior and then some.

    DomainEntityScriptId(Or<DomainContext<P>, Or<EntityScriptIdContext<P>, Or<ScriptIdContext<P>, SharedContext<P>>>>),

You can read the Ors as short-circuiting like this:

1. If there is a domain, DomainContext provides a context, or
2. if there is an entity and script, EntityScriptIdContext is used, or
3. if there is a script (which is the case for StaticScripts that have no entity), ScriptIdContext is used, or
4. if there is no domain, entity, or script, then SharedContext is used.

The prior version had behaviors 2 and 3 and 4 as a separate assignment mode. This adds domains without interfering with the rest if they're not used.

And its easy to design different behaviors within the library. For instance, if we wanted to have all static scripts live in the same context we can drop the ScriptIdContext.

    DomainX(Or<DomainContext<P>, Or<EntityScriptIdContext<P>, SharedContext<P>>>),

This means SharedContext will provide contexts for any context key that only has a script ID like static scripts. (We could also make static scripts work with domains as an alternative that's more flexible.)

Reify me not

I did a big refactor on the event_handler_inner<P: IntoScriptPluginParams> function to make it work with the ScriptContext. I chose some code duplication for hopefully better runtime performance. Is there any performance tests I could exercise to compare with the previous version?

It now has these benefits:

• Early exits
• Does not query and reify all scripts to a collection every call
• Queries one entity only when necessary
• Queries scripts only when necessary

Apologies

I am sorry that this PR has gotten so big. At the beginning it seemed like this could be a less invasive set of changes. Having failed to make this change set small, I have endeavored to make it good.

I updated the BMS book's chapter on contexts and managing scripts.

I realized through the original documentation that I had misunderstood BMS's original script context isolation. I thought it somehow isolated scripts on an entity-script pair basis. I wrote the entity-script pair context so it would be a drop-in substitute for the original system. Oops. Turns out isolating based on script would be the drop-in replacement.

Choose ScriptContext::default()

Now's the time to choose the default.

• Use ScriptContext::per_script().with_domains(). This is the closest to the preceding version's behavior.
• Use ScriptContext::per_entity_and_script().with_domain(). This is maximally isolated script execution, and perhaps less surprising since the scripts don't share a context between entities.

The default is currently set to the latter.

Migration Guide

I plan to also write a chapter on migrating from one version to another imitating Bevy's excellent migration guides.

Ello, apologies for not interacting much yet, rest assured I am observing the branch and trying to grok it (and I like this direction), I've now gotten the bevy migration branch to a final state, so now I am going to be reviewing the changes in this PR over the next week or so, do you mind merging the last bit of conflicts (as I need the xtask changes to load the workspace locally).

for the merge order, now I am thinking:

• release from this branch for a handles+bevy0.15 version via staging, perhaps as an alpha
• merge bevy 0.16 migration to main, release that as no-handles+bevy0.16
• rebase this branch against that, to get the handles+bevy0.16 version

thoughts?

Great to hear!

I think the versions sound fine. A handles+bevy0.15 as an alpha is good by me, better even than polluting the version space with use-handles and not.

rebase this branch against that [no-handles+bevy0.16], to get the handles+bevy0.16 version

I think the desired end is good, but I worry the process of rebasing this branch onto a 0.16 will be a chore. But maybe it won't. To update this branch to "main," I'm going to squash everything in this branch just to make the rebasing easier. Maybe it won't be as troublesome if the commits are fewer, but I was initially thinking I'd take the patch from the old handles+0.16 and apply that on top of this branch with whatever fix up was required.

I squashed this branch and rebased it to main. It's compiling. There are two tests which are failing.

failures:
    commands::test::test_commands_with_default_assigner
    commands::test::test_commands_with_global_assigner

And I'm not seeing the unload and reload work yet. Probably need to add a test for those.

I think the desired end is good, but I worry the process of rebasing this branch onto a 0.16 will be a chore. But maybe it won't. To update this branch to "main," I'm going to squash everything in this branch just to make the rebasing easier. Maybe it won't be as troublesome if the commits are fewer, but I was initially thinking I'd take the patch from the old handles+0.16 and apply that on top of this branch with whatever fix up was required.

Ah yeah sorry by rebase I mean rebase OR merge, don't mind which, squash and rebase is also good! Folks at work treat those words interchangably and it's infected me

Nice, I will pull this down locally and play with it

Fixed the failing assignment tests. All tests are passing!

I added an example called "run-script" to exercise the reload functionality. It's a mouthful to run reload.lua because of the needed features:

cargo run --features lua54,bevy/file_watcher,bevy/multi_threaded --example run-script -- reload.lua

I know you're looking to get more examples, so I figured this might be good to have for the rust part and maybe putting together quick pure lua or rhai examples as well.

Reload functionality is not working yet in this branch.

One clippy warning remains but I don't know why. It refers to an "output" variable that isn't there. See last commit for the exact log.

🚧 WIP: Review in progress🚧

Alright, the PR changes quite a few things I am going to use the following convention in my reviews:

• 🤷 Things of note, not requesting any changes / starting a discussion that might lead to changes requested, would like to hear your thoughts
• 🛑 Things that will block merging, I would like to see changes before going further.

And do keep in my mind I am happy to make those changes, so do let me know if you want me to!

I'll also divide my thoughts into the areas that were touched:

pub(crate) enum ScriptAssetEventKind {
    Added,
    Attached,
    Detached,
    Removed,
    Modified,
}

Thanks for taking the time to look over it.

Script asset events

I believe this was removed in this PR since the metadata synchronising is no longer necessary

That was the reason, but I think you're right that ScriptEvents should be reconstituted especially since now the events are not aligned with the AssetEvent.

Script evaluation queue feels unnecessary

It started as a system Local before I was compelled to pull it out for static scripts.

I realized too that the queue isn't necessary unless you're putting scripts into the same context.

I will attempt to do something as you suggest but I admit I'm foggy on what that will entail.

ScriptContext enum is very complex

That's a really good point. It's perhaps a labor of understanding that we can now schluff off, but I think you're right. We could get the same effect by having a HashMap<ContextKey, P::C> and applying some filter to the ContextKey before hand:

fn shared(context_key: ContextKey) -> ContextKey {
    ContextKey::default()
}

fn per_script(context_key: ContextKey) -> ContextKey {
    ContextKey {
      script: context_key.script,
      ..default()
    }
}

Oh, I see perhaps you'd suggest Fn(&ContextKey) -> ContextId rather. It does seem like ContextId wants to be expressed somehow.

I kind of liked how script handles were a proxy for context handles—when all script handles were dropped, the context was dropped—and it has made me wonder if Contexts ought to be assets with their own handles. [Shrug.]

The Domain concept you've come up with then appears for free, as you can map to arbitrary ID's based on the set of keys we make available

Hmm, I don't quite follow but I'm intrigued by the idea. What's the type of ContextId in your mind?

(assuming you'd be driving domain off the asset paths etc)

I hadn't thought domains would be related to asset paths. Best example I have for their potential use is what I used in the book example:

commands.spawn((
    ScriptComponent(vec![asset_server.load("player.lua")]),
    ScriptDomain(Domain::new("player")),
)).with_children(|parent| {
    parent.spawn((
        ScriptComponent(vec![asset_server.load("sword.lua")]),
        ScriptDomain(Domain::new("player")),
    ));
})

I completely reworked the ScriptContext, and I think it's much better now. I dropped all those ScriptContextProvider modules. I dropped the ScriptContextProvider trait. Took out a lot of lines of code. Yay!

We use a simple enum ContextRule that allows for custom rules. The user can now configure policies much more flexibly, e.g., [ContextRule::Domain, ContextRule::Custom(custom_context_selection_fn), ContextRule::Script, ContextRule::Shared].

I've reintroduced the ScriptEvent, and ScriptQueue has been demoted to a Local and type alias (and this incidentally fixed a latent bug that would have become apparent when multiple runtimes were mucking with one ScriptQueue).

I haven't sorted out the tests yet.

There is a complication with using the ContextKey directly as the key in ScriptContext: Because the script field is an Option<Handle<ScriptAsset>>, I worry that a strong handle vs a weak handle for the same asset may not hash or == as we might like. I need to either ensure we only store weak handles or validate that the operations for search and insert won't depend on the handle being strong or weak. In my 'use-handles-dev' I have a commit that changes ContextKey.script to be a Option<ScriptId> but then we'll lose a lot of the nicer error reporting that can show you the path of the script during logging. EDIT: Handles are hashed and == on their IDs, so no problem there!

All right. This is hopefully looking more like what you asked.

DeleteScript is kind of doing double duty. It deletes a static script and it deletes a context. Perhaps a DeleteContext should be extracted out from it. I don't know.

Also since we're storing ContextKeys as our key, I'll just note I'm only storing weak handles for scripts.

Looking really good! The events make this extremely easy to read!

Starting with this branch, I re-applied Alex Parlett's 0.16 patch which is now on available on use-handles-0.16.

I am just working out the last few kinks, namely:

• we've lost the concept of "script residency", i.e. we can't ask if a script is loaded currently, this is the reason teh integration tests didn't work with the asset loader, the "is loaded" condition was taken out of the loop
• fixing rhai bits and bobs
• something going on with the benchmarks

Re: script residency. [Looks at the test harness lib.rs:305 file.] Oh, that's right. I imagine you're looking at this code:

        if let Some(event) = error_events.into_iter().next() {
            // eprintln!("XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXx");
            // if ! app.world().resource::<AssetServer>().load_state(&handle).is_loaded() {
            //     continue;
            // }
            return Err(event
                .error
                .display_with_world(WorldGuard::new_exclusive(app.world_mut())));
        }

I commented that out, but the script is being added a few lines above lib.rs:244.

    // The following code did not work, possibly because of the asynchronous
    // nature of AssetServer.
    //
    // ```
    // let handle = app.world_mut().resource_mut::<AssetServer>().load(&script_path);
    // app.world_mut().spawn(ScriptComponent::new([handle.clone()]));
    // ```
    let handle = {
        let mut script_dir = manifest_dir.clone();
        script_dir.push("assets");
        script_dir.push(script_id.path());
        // Read the contents and don't do anything async.
        let content = fs::read_to_string(&script_dir)
            .map_err(|io| format!("io error {io} for path {script_dir:?}"))?;
        let mut script = ScriptAsset::from(content);
        script.language = P::LANGUAGE;
        app.world_mut()
            .resource_mut::<Assets<ScriptAsset>>()
            .add(script)
    };

I was getting intermittent behavior if I recall correctly, so I figured if I could make script loading synchronous rather than async, it would be more reliable.

oh yeah I get that, in the integration test though I'd ideally like to run the pipeline from start to end, for unit tests that makes a lot of sense.

I haven't quite gotten there this weekend as I'd hoped, I've bumped into a few more things I haven't noticed before (apologies, don't worry about patching to 0.16, I can continue from here), still have to cook a little longer, but the current TL;DR of what I am working on is:

• Introducing ScriptAttachment enum, (i.e. EntityScript, StaticScript), which acts as the context policy "input", this lets us ask questions like, how many scripts live together in the same context
  • thanks to that we can automatically tell if a context needs deleting (if we just detached the last script living in the context, delete it)
  • we can also ask if a script has actually loaded (without this, in a shared context scenario, you will only know if at least one script has loaded into the shared context, but not THIS script)
• sort of overhauling the Recipients enum, as I don't think Script and Entity fit the model that well anymore, might be best to shift the querying onto the user, and provide individual ScriptEntity and StaticScript as well as AllScripts and AllContexts (and Domain) variants, if people need to send more variations, they can simply query the ECS before sending, and if we want to batch, we can improve on this internally later
• I see a lot of potential for simplification in this new model (I think it's even possible to just stop querying anything apart from the existing contexts in the event handler, since it only cares about what's actually loaded, i.e. is resident in a context), not sure if that possibility existed before, or if it's opened up just now, but might as well simplify now. If we want to notify users of unloaded scripts we can use a separate system with warnings
• I think I'll also take this opportunity to write a slightly better harness for testing complex loading scenarios in an integration context, think scenario.json with list of events that happen, current context policy etc, since the complexity sort of blew up in this area, I'd like to improve the testing a bit
• I am also experimenting with on_add and on_remove component hooks, to simplify the syncing process (let's you see what the removed component was before it's removed)

Haven't said it enough yet, but your work is much appreciated, and got me thinking a lot! Had I been implementing this from the start I would probably involve auto-incrementing u64's for context ID's and lots more bookkeeping, so it's great to get a new perspective! I am still thinking about your Or<A,B> concept for hashmaps and if I can use that pattern to simplify things elsewhere.

let me know if there's anything concerning in the above list, or if I missed something!

Thanks! I appreciate the kind words. I'm happy to have you take the reigns. I felt like I had been pretty intrusive with my changes, so I tried not to upset the apple cart where I could, but I'm glad you're reconsidering some things like Recipient, which doesn't quite cut along nice lines like it used to. So if you can make the recipients cut along the new lines which will be performant, I think that's a good change.

The only things I'd want preserved are handles for scripts, in-order loading (but it could be relaxed to be in-order per context), and a notion of domains. (I realized with Nano-9 if I use a "nano-9" domain instead of a shared context then I wouldn't have to monopolize BMS, and the user would still be free to use BMS in any fashion they like, which feels like a win.)

I look forward to seeing your changes. Happy to test and review when it's ready.